Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Settings - Fixes for whitespaces and comments, added unit tests #1622

Merged
merged 2 commits into from
Oct 24, 2023
Merged

Settings - Fixes for whitespaces and comments, added unit tests #1622

merged 2 commits into from
Oct 24, 2023

Conversation

johnb432
Copy link
Contributor

@johnb432 johnb432 commented Oct 21, 2023 •
edited
Loading

When merged this pull request will:

  • Title. More specifically it should address Latest CBA version does not honor white space in the CBA settings Configuration File for Task Force Arrowhead Radio (BETA) #1619 and Some CBA Settings fail to import #1620.
  • Comments fix: From line 29 through 34.
    • If the order of the regexReplace commands is switched (so removing multiline comments first, then single line), it will not work. This makes be believe both my regex' are not robust enough.
    • There are cases I have thought of that could be problematic, but I'm not sure how to test, because the .editorconfig file restricts them. E.g. having a single line comment be at the end of a file, but not having a newline (is that even possible?) or using different newline encoding styles (LF, CR and CRLF).
  • Whitespace fix: Removal of lines 25 through 29. The comment was mostly wrong, except in one instance:

If in single quotes, it removed the whitespaces (this is the old code):
' T E S T ' becomes "TEST"

Single quoted strings will be converted into double quoted strings.
So ' T E S T ' will remain/become " T E S T ".

I'd appreciate any help.

@johnb432 johnb432 changed the title Settings - Fixes for whitespace and comments and unit tests Settings - Fixes for whitespaces and comments, added unit tests Oct 21, 2023
@jonpas jonpas added this to the 3.16.1 milestone Oct 21, 2023
@jonpas jonpas added the Bug Fix label Oct 21, 2023
jonpas
jonpas reviewed Oct 21, 2023
View reviewed changes
addons/settings/test_parse.sqf
Comment on lines +60 to +73
["test1", "", 0],
["test2", "", 0],
["test3", " T E S T " , 0],
["test4", " T E S T ", 0],
["test5", "[ ' t e s t ' , "" T E S T "" ]", 0],
["test6", "[ "" t e s t "" ,

"""" T E S T """" ]", 0],
["test7", "[ true, false ]", 0],
["test8", "[ "" item_1 "" , "" item_2 "" ]", 0],
["test9", "[ ' item_1 ' , ' item_2 ' ]", 0],
["test10", "[ ' item_1 ' , "" item_2 "" ]", 0],
["test11", "[ '"" item_1 ""' , ' item_2 ' ]", 0],
["test12", "[ ' item_1 ' , ""' item_2 '"" ]", 0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tabs!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tabs!

Tabs inside or outside of the strings? I saw the github action complained about tabs, but I want the tests to run with tabs (to make sure they work - unless that's unnecessary?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I see now. @PabstMirror do we have a way to ignore validating parts of code?

I don't think we need to test for tabs though, as whitespace is whitespace to Arma, but I may be wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passing CI now

@johnb432
Copy link
Contributor Author

johnb432 commented Oct 23, 2023 •
edited
Loading

Something I forgot to mention explicitly for the whitespace fix regarding single quoted strings:
They will no longer have their whitespace removed. Plus, they will be converted into double quoted strings.
So ' T E S T ' will remain/become " T E S T ".

I'm going to re-test it though, to be 100 % certain, as the tests above were affected with Advanced Developer Tools. Confirmed.

@jonpas jonpas merged commit 71a08bd into CBATeam:master Oct 24, 2023
4 checks passed
This was referenced Oct 24, 2023
Closed
Closed
@johnb432 johnb432 deleted the settings-fixes branch August 6, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PabstMirror PabstMirror PabstMirror left review comments

@jonpas jonpas jonpas approved these changes

Assignees
No one assigned
Labels
Bug Fix
Projects
None yet
Milestone
3.16.1
Development

Successfully merging this pull request may close these issues.
3 participants
@johnb432 @jonpas @PabstMirror